feat!: Replace isAccessibleCallback with diagnosticsSummaryCallback in DoenetEditor#1035
Conversation
Co-authored-by: Copilot <copilot@github.com>
🦋 Changeset detectedLatest commit: 023d2d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR updates the DoenetML editor API by replacing the boolean isAccessibleCallback with a more general diagnosticsSummaryCallback that reports diagnostic counts across categories, and wires it through DoenetEditor → EditorViewer. It also updates Cypress coverage to validate the new callback payload.
Changes:
- Replace
isAccessibleCallback(isAccessible: boolean)withdiagnosticsSummaryCallback(summary)inDoenetEditor/EditorViewer. - Extend
mergeDiagnosticsByTypeto returnwarningsCount,errorsCount, andinfosCountalongside accessibility counts. - Update Cypress tests and the Cypress test harness to read/verify the new summary object (including new warning/error tests).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-cypress/src/CypressTest.tsx | Update Cypress harness to capture the new diagnostics summary callback payload. |
| packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js | Update/add Cypress assertions to validate new callback shape and counts. |
| packages/doenetml/src/doenetml.tsx | Replace public DoenetEditor prop with diagnosticsSummaryCallback and pass through to EditorViewer. |
| packages/doenetml/src/EditorViewer/diagnostics.ts | Add warning/error/info count fields to merged diagnostics result. |
| packages/doenetml/src/EditorViewer/EditorViewer.tsx | Invoke diagnosticsSummaryCallback only after viewer diagnostics have been received. |
| .changeset/diagnostics-summary-callback.md | Add changeset describing the callback replacement and behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Updates the Doenet editor/viewer diagnostics API by replacing the boolean isAccessibleCallback with a richer diagnosticsSummaryCallback that reports counts across diagnostic categories, and adjusts Cypress tests/harness to validate the new behavior.
Changes:
- Replaced
isAccessibleCallbackwithdiagnosticsSummaryCallbackonDoenetEditorandEditorViewer, emitting counts for warnings/errors/infos and accessibility levels. - Extended
mergeDiagnosticsByTypeto returnwarningsCount,errorsCount, andinfosCountalongside existing grouped diagnostics. - Updated Cypress test harness and E2E accessibility/diagnostics tests to use the new callback shape.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-cypress/src/CypressTest.tsx | Switches Cypress harness from isAccessibleCallback to diagnosticsSummaryCallback and exposes the latest summary via window.returnDiagnosticsSummaryCallbackValue(). |
| packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js | Updates/expands E2E assertions to validate the new diagnostics summary counts for accessibility, warnings, and errors. |
| packages/doenetml/src/doenetml.tsx | Updates DoenetEditor public prop type and forwards diagnosticsSummaryCallback into EditorViewer. |
| packages/doenetml/src/EditorViewer/diagnostics.ts | Introduces DiagnosticsSummary type and adds warnings/errors/infos counts to mergeDiagnosticsByType return value. |
| packages/doenetml/src/EditorViewer/EditorViewer.tsx | Adds gating to avoid emitting summary before viewer diagnostics arrive and emits the new summary payload. |
| .changeset/diagnostics-summary-callback.md | Adds release note for the callback replacement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses two PR review comments: 1. Memoize the diagnosticsSummary object in EditorViewer via useMemo so its reference is stable when counts don't change, avoiding potential render loops when consumers store the object in state. 2. Reset diagnosticsSummaryRef in CypressTest when doenetMLstring changes, ensuring stale summary values from previous test runs aren't returned to subsequent tests.
There was a problem hiding this comment.
Pull request overview
This PR replaces the isAccessibleCallback API on DoenetEditor/EditorViewer with a richer diagnosticsSummaryCallback that reports counts across diagnostic categories, and updates diagnostics aggregation + Cypress coverage accordingly.
Changes:
- Replace
isAccessibleCallback(isAccessible: boolean)withdiagnosticsSummaryCallback(diagnosticsSummary)acrossDoenetEditorandEditorViewer. - Extend
mergeDiagnosticsByTypeto returnwarningsCount,errorsCount, andinfosCountin addition to accessibility counts. - Update Cypress harness + E2E tests to assert the new callback payload (including warning/error counts).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-cypress/src/CypressTest.tsx | Switch Cypress harness window hook and editor prop wiring to the new diagnostics summary callback. |
| packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js | Update and expand E2E assertions to validate the new summary shape and counts. |
| packages/doenetml/src/doenetml.tsx | Update DoenetEditor public prop surface to accept diagnosticsSummaryCallback. |
| packages/doenetml/src/EditorViewer/diagnostics.ts | Introduce DiagnosticsSummary type and extend mergeDiagnosticsByType to compute counts. |
| packages/doenetml/src/EditorViewer/EditorViewer.tsx | Emit the new diagnostics summary via callback (with a “received from viewer” gate). |
| .changeset/diagnostics-summary-callback.md | Document the API replacement in Changesets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cqnykamp
left a comment
There was a problem hiding this comment.
Looks good as far as I can tell.
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Doenet editor diagnostics callback API by replacing the boolean isAccessibleCallback with a richer diagnosticsSummaryCallback that reports counts for multiple diagnostic categories, and updates Cypress tests accordingly.
Changes:
- Replace
isAccessibleCallbackwithdiagnosticsSummaryCallbackonDoenetEditor/EditorViewer. - Extend
mergeDiagnosticsByTypeto also return warning/error/info counts and emit a memoizedDiagnosticsSummary. - Update Cypress test harness and e2e tests to assert the new callback shape and behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-cypress/src/CypressTest.tsx | Updates the test harness to expose diagnostics summary via a window callback and resets it on new DoenetML. |
| packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js | Updates/extends Cypress assertions to validate the new summary counts (accessibility, warnings, errors). |
| packages/doenetml/src/doenetml.tsx | Swaps DoenetEditor prop from isAccessibleCallback to diagnosticsSummaryCallback and wires it through to EditorViewer. |
| packages/doenetml/src/EditorViewer/diagnostics.ts | Adds DiagnosticsSummary type and returns warning/error/info counts from mergeDiagnosticsByType. |
| packages/doenetml/src/EditorViewer/EditorViewer.tsx | Implements gated + memoized diagnosticsSummaryCallback invocation based on viewer diagnostics receipt. |
| .changeset/diagnostics-summary-callback.md | Adds a changeset entry describing the callback replacement and new payload shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Replaces the
isAccessibleCallbackprop onDoenetEditor(and the underlyingEditorViewer) with a newdiagnosticsSummaryCallbackprop.Before
The callback fired with a single boolean indicating whether there were no level-1 accessibility issues.
After
The callback now fires with an object containing counts for all diagnostic categories:
Additional changes
mergeDiagnosticsByTypenow also returnswarningsCount,errorsCount, andinfosCount.